Return errors instead of panics #638
Conversation
|
@microsoft-github-policy-service agree |
| program := l.GetProgram() | ||
| file := program.GetSourceFile(fileName) | ||
| if file == nil { | ||
| panic("file not found") |
There was a problem hiding this comment.
My understanding is that these are mainly invariants that shouldn't be broken, such that we'd want to know about them. But, maybe we need to surface these differently (like gopls' bug.Report), or as a general panic handler (sort of like old TS with exceptions, which we also did not use except in tragic cases).
There was a problem hiding this comment.
Yeah, but it happened in the mentioned issue, and I'm not sure it should crash the LSP server.
I don't have any strong opinion, because it's always up to the team and the project. But in general, panics are for unexpected errors, this one (even if it should never happen in real life) is kinda expected.
And I'm not a big fan of global recovery exactly because if some panic happens, we'd want to know about it.
I think the same problem would be reported in issue even without panic anyway, since it's a bug (at least looks like the one).
There was a problem hiding this comment.
I agree, lsp servers shouldn't crash for file not found cases. At least the ts lsp , go lsp doesn't crash. I think we need an error channel to manage errors async. The Current version #645 seems to be halting at every error.
There was a problem hiding this comment.
I think I generally disagree with this, at least for now; operations on the Program should be infalliable, more or less. It's a bug in the Project/ProjectService if it ever attempts to look up a file that doesn't exist.
For the other LSP calls, I can imagine us wanting to do error returns (especially for cancellation), but I think that will probably need to be done at a later point.
tsgo heavily overuses panics, that can lead to the situations mentioned in #631:
PR is intended to start moving the codebase towards more idiomatic error handling.
Atm, I've changed 2 panics and we probably need to agree on the approach: I can keep adding changes to this PR, but it'll be big and hard to follow, or I can create a separate PRs to keep them small and simple.